Skip to content

List skipped extensions in run-tests.php #8363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

List skipped extensions in run-tests.php #8363

wants to merge 13 commits into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Apr 13, 2022 •
edited
Loading

discovered in #8348 - Tests skipped because of missing ext were listed nowhere, this change improves the testing output by listing the skipped exts.

discovered in #8363 (comment) - The

php-src/run-tests.php

Line 890 in c809a21

if (!extension_loaded($ext) && @dl($file)) {
was producing silently ignored! fatal error due #9196 (as the loaded exts were echoed at the start, the problem remained hidden, with this PR, we echo at the end, and fatal error will imply no loaded ext will be detected at all - easy to notice any problem)

also, we centralize the place where we normalize the loaded ext names to ext so/dll filenames

@mvorisek
Copy link
Contributor Author

why is zend_test listed there? can skeleton be build/tested as well?

@cmb69
Copy link
Member

cmb69 commented Apr 13, 2022

zend_test is a real extension (used to be able to test stuff which would otherwise be untestable). skeleton is just a template for an extension, and as such cannot be build.

@mvorisek
Copy link
Contributor Author

zend_test is a real extension (used to be able to test stuff which would otherwise be untestable)

I know, but why it is listed as untested?

skeleton is just a template for an extension, and as such cannot be build.

if we would replace %EXTNAMECAPS% with SKELETON (and %EXTNAME% and similar template vars)?

@cmb69
Copy link
Member

cmb69 commented Apr 14, 2022

I know, but why it is listed as untested?

Likely because zend_tests is not loaded, when $exts_to_test is initialized. I think this might be solved by targeting the master branch, what such improvements should do anyway.

if we would replace %EXTNAMECAPS% with SKELETON (and %EXTNAME% and similar template vars)?

Why not simply exclude this extension from those stats?

@mvorisek
Copy link
Contributor Author

I will look what I can do for master, can this PR be merged to PHP 8.0 as is?

@cmb69
Copy link
Member

cmb69 commented Apr 14, 2022

I don't think this should go into any of the stable branches.

@mvorisek mvorisek marked this pull request as draft April 16, 2022 20:03
@mvorisek mvorisek changed the base branch from PHP-8.0 to master April 16, 2022 20:04
@mvorisek mvorisek force-pushed the list_skipped_exts branch 2 times, most recently from f60c84d to df85e15 Compare April 16, 2022 22:05
@mvorisek mvorisek changed the title List skipped extensions explicitly in test summary List skipped tests even if ext is unloadable Apr 16, 2022
@mvorisek mvorisek changed the title List skipped tests even if ext is unloadable List tests as skipped even if extension is unloadable Apr 16, 2022
@mvorisek mvorisek force-pushed the list_skipped_exts branch 6 times, most recently from ffcd710 to d308f55 Compare April 17, 2022 09:34
@mvorisek mvorisek changed the title List tests as skipped even if extension is unloadable List tests as skipped even if extension is not loadable Apr 17, 2022
@mvorisek mvorisek force-pushed the list_skipped_exts branch from 0ffd3e8 to c6a4df8 Compare April 17, 2022 10:41
@mvorisek mvorisek force-pushed the list_skipped_exts branch 2 times, most recently from ecd0d78 to a682d60 Compare September 10, 2022 09:29
@mvorisek mvorisek force-pushed the list_skipped_exts branch 3 times, most recently from 63f95e6 to b50ab03 Compare September 10, 2022 09:52
@mvorisek mvorisek changed the title List tests as skipped even if extension is not loadable List skipped extensions in run-tests.php Sep 10, 2022
@cmb69
Copy link
Member

cmb69 commented Sep 12, 2022

Also please have a look at https://ci.appveyor.com/project/php/php-src/builds/44737578/job/tg57sktu8c5i6oyl#L1690. The ext\opcache\tests should be executed for this configuration.

@mvorisek
Copy link
Contributor Author

mvorisek commented Sep 12, 2022 •
edited
Loading

Also please have a look at https://ci.appveyor.com/project/php/php-src/builds/44737578/job/tg57sktu8c5i6oyl#L1690. The ext\opcache\tests should be executed for this configuration.

image

there are two jobs, opcache should probably be NOT executed for OPCACHE=0 job

but good catch, the OPCACHE=1 job is skipping all exts [1], do you have any idea why? Does dl() supports ZTS?

[1] https://ci.appveyor.com/project/php/php-src/builds/44737578/job/5e6uc64ft1vl4cty#L1706

@mvorisek mvorisek marked this pull request as draft September 12, 2022 12:11
@mvorisek mvorisek force-pushed the list_skipped_exts branch 3 times, most recently from 27d32e6 to c555755 Compare September 13, 2022 16:40
@mvorisek mvorisek marked this pull request as ready for review September 13, 2022 18:43
@mvorisek
Copy link
Contributor Author

PR is done

@mvorisek
Copy link
Contributor Author

@cmb69 adjusted for your #9557, if there are no more objections, please squash into one commit and merge

@cmb69
Copy link
Member

cmb69 commented Sep 26, 2022

Thank you!

@cmb69 cmb69 closed this in baef47e Sep 26, 2022
@mvorisek mvorisek deleted the list_skipped_exts branch September 26, 2022 09:16
@iluuu1994
Copy link
Member

This causes the following error for me:

Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /home/ilutov/Developer/php-src/run-tests.php:3054
Stack trace:
#0 /home/ilutov/Developer/php-src/run-tests.php(726): compute_summary()
#1 /home/ilutov/Developer/php-src/run-tests.php(4019): main()
#2 {main}
  thrown in /home/ilutov/Developer/php-src/run-tests.php on line 3054
make: *** [Makefile:209: test] Error 255

where $ignored_by_ext is NULL.

@cmb69
Copy link
Member

cmb69 commented Sep 26, 2022

Hmm, on a quick glance, I think we need to initialize $ignored_by_ext earlier. I'll have a closer look tomorrow.

@cmb69
Copy link
Member

cmb69 commented Sep 27, 2022

#9617 should fix the issue.

@SakiTakamachi SakiTakamachi mentioned this pull request Dec 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 left review comments

Assignees
No one assigned
Labels
Category: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvorisek @cmb69 @iluuu1994 @ramsey